Skip to content
This repository was archived by the owner on Sep 12, 2019. It is now read-only.

Fix dev command when running with npm - fix #119 #170

Merged
merged 3 commits into from
May 3, 2019
Merged

Fix dev command when running with npm - fix #119 #170

merged 3 commits into from
May 3, 2019

Conversation

stefanjudis
Copy link
Contributor

@stefanjudis stefanjudis commented May 3, 2019

Make defining npm scripts other than start possible in dev command

The behavior was already described in #119. When using npm and defining configuring another command than npm start in the command field the script execution fails. This is because currently the command get's inserted an additional run argument when npm was detected.

Exception:

Waiting for localhost:7894.npm ERR! missing script: run

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/sjudis/.npm/_logs/2019-05-03T19_11_15_952Z-debug.log

FAIL: 1

I believe that it'd be better to always pass the arguments through to execution because this is what users are used to from the build section of the netlify.toml.

- Test plan

To test this I added a netlify.toml file to this repo as follows.

# example netlify.toml
[dev]
  command = "npm run version"
  port = 7894

Running $ ./bin/run dev leads then to the exception because it's running npm run run version.

🐧  ➡ ./bin/run dev                                                                   [21:10:54]
◈ Netlify Dev ◈
◈ Starting Netlify Dev with undefined
Waiting for localhost:7894.npm ERR! missing script: run

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/sjudis/.npm/_logs/2019-05-03T19_11_15_952Z-debug.log

FAIL: 1

After my changes, the command succeeds.

- Description for the changelog

  • Fix npm scripts in dev command

- A picture of a cute animal (not mandatory but encouraged)
duck

@swyxio
Copy link
Contributor

swyxio commented May 3, 2019

i appreciate the PR and it is an extremely cute duck but i dont think this is the right place to fix this. it breaks other cases (eg our detector logic). so i rather fix this at the netlify.toml parsing level.

@swyxio
Copy link
Contributor

swyxio commented May 3, 2019

can you confirm if my updates work for you @stefanjudis ?

@stefanjudis
Copy link
Contributor Author

That works. :)

(Yeah - I don't have the whole picture and just went into the most obvious one. 🙈)

@swyxio swyxio merged commit 12f274f into netlify:master May 3, 2019
@swyxio
Copy link
Contributor

swyxio commented May 3, 2019

thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants